router: inherit upstream connection filter state in upstream request stream info by snowp · Pull Request #10458 · envoyproxy/envoy

snowp · 2020-03-19T20:33:30Z

This adds the upstream connection filter state to the upstream request stream info, making it possible to look up upstream connection state from upstream HTTP access logs.

Risk Level: Low
Testing: UT
Docs Changes: n/a
Release Notes: n/a
Fixes #10457

…r state Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

snowp · 2020-03-19T21:48:40Z

include/envoy/stream_info/stream_info.h

   */
-  virtual const FilterStateSharedPtr& filterState() PURE;
-  virtual const FilterState& filterState() const PURE;
+  virtual const FilterStateSharedPtr& filterState() const PURE;


This change is a bit unfortunate; it's to make it possible to get the parent FilterStateSharedPtr from a const handle. Open for suggestions here for how to do this better

I'd rather have a const-cast somewhere; this looks on the surface like it is const-safe, but the returned ptr is non-const, even though the method is const.

Actually, I think I would make onPoolReady() pass a non-const StreamInfo. I think it's reasonable for that function to have non-const access to the StreamInfo.

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

ggreenway · 2020-03-20T16:03:53Z

include/envoy/stream_info/stream_info.h

   */
-  virtual const FilterStateSharedPtr& filterState() PURE;
-  virtual const FilterState& filterState() const PURE;
+  virtual const FilterStateSharedPtr& filterState() const PURE;


I'd rather have a const-cast somewhere; this looks on the surface like it is const-safe, but the returned ptr is non-const, even though the method is const.

ggreenway · 2020-03-20T16:14:36Z

include/envoy/stream_info/stream_info.h

   */
-  virtual const FilterStateSharedPtr& filterState() PURE;
-  virtual const FilterState& filterState() const PURE;
+  virtual const FilterStateSharedPtr& filterState() const PURE;


Actually, I think I would make onPoolReady() pass a non-const StreamInfo. I think it's reasonable for that function to have non-const access to the StreamInfo.

ggreenway · 2020-03-20T16:18:59Z

test/common/router/router_test.cc

+  bool filter_state_verified = false;
+  router_.config().upstream_logs_.push_back(
+      std::make_shared<TestAccessLog>([&](const auto& stream_info) {
+        filter_state_verified = stream_info.upstreamFilterState()->hasDataWithName("foo");


nit: would slightly increase readability if the string was "upstream data" or similar.

ggreenway · 2020-03-20T16:20:55Z

/wait

stale · 2020-03-27T17:02:52Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

sunjayBhatia · 2020-03-31T15:02:57Z

source/extensions/access_loggers/grpc/grpc_access_log_utils.cc


  for (const auto& key : config.filter_state_objects_to_log()) {
-    if (stream_info.filterState().hasDataWithName(key)) {
+    if (stream_info.filterState()->hasDataWithName(key)) {


It appears #9845 added this ambiguity causing the windows CI compilation failures, not yet quite sure why MSVC is able to resolve it here: https://github.com/envoyproxy/envoy/pull/9845/files#diff-a5570d8b9053a47a1547728cb6c90989 and not this case

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

snowp · 2020-04-01T15:24:50Z

/retest

repokitteh-read-only · 2020-04-01T15:24:55Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10458 (comment) was created by @snowp.

see: more, trace.

snowp · 2020-04-02T18:13:11Z

/azp run

azure-pipelines · 2020-04-02T18:13:22Z

Azure Pipelines successfully started running 1 pipeline(s), but failed to run 2 pipeline(s).

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

ggreenway · 2020-04-06T18:51:03Z

CI is unhappy :(

/wait

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

snowp · 2020-04-07T18:07:02Z

Seems to be OOMing in CI, merging again to pick up #10669

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

snowp · 2020-04-17T20:14:04Z

@ggreenway I think this can be reviewed now, the failing PR run seems to be due to general OS X issues in CI

ggreenway

LGTM

…stream info (envoyproxy#10458) Signed-off-by: Snow Pettersen <kpettersen@netflix.com> Signed-off-by: pengg <pengg@google.com>

…ream info (#15543) This makes it possible to lookup upstream connection filter state from a downstream HTTP access log, similar to #10458 which does the same for upstream HTTP access logs. Signed-off-by: Elisha Ziskind <eziskind@google.com>

Snow Pettersen added 2 commits March 19, 2020 13:26

inherit upstream connection filter state when creating upstream filte…

8dd290c

…r state Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

update references to lifespan enum

dcf9928

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

snowp requested a review from lizan as a code owner March 19, 2020 20:33

Snow Pettersen added 2 commits March 19, 2020 13:58

filterState() now returns a pointer

736c181

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

more ptr access

0a4d2d1

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

mattklein123 assigned ggreenway Mar 19, 2020

remove debug logs

0f7901c

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

snowp commented Mar 19, 2020

View reviewed changes

remove redundant mocks

e436589

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

ggreenway requested changes Mar 20, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 20, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 27, 2020

snowp removed the stale stalebot believes this issue/PR has not been touched recently label Mar 30, 2020

Snow Pettersen added 2 commits March 30, 2020 09:48

Merge remote-tracking branch 'envoy/master' into upstream-filter-state

be43ddc

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

pass non-const handle in onPoolReady

7d37607

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

repokitteh-read-only bot removed the waiting label Mar 31, 2020

Merge remote-tracking branch 'envoy/master' into upstream-filter-state

618134e

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

sunjayBhatia reviewed Mar 31, 2020

View reviewed changes

Snow Pettersen added 2 commits March 31, 2020 10:40

Merge remote-tracking branch 'envoy/master' into upstream-filter-state

96ea29e

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

revert grpc access log

cae7090

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

Snow Pettersen added 2 commits April 2, 2020 12:42

Merge remote-tracking branch 'envoy/master' into upstream-filter-state

a12c57a

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

remove test specific config() call - the real class has one now

70bade8

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

repokitteh-read-only bot added the waiting label Apr 6, 2020

Merge remote-tracking branch 'envoy/master' into upstream-filter-state

3c91db9

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

repokitteh-read-only bot removed the waiting label Apr 6, 2020

Merge remote-tracking branch 'envoy/master' into upstream-filter-state

2bb4245

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

Snow Pettersen added 3 commits April 8, 2020 16:14

Merge remote-tracking branch 'envoy/master' into upstream-filter-state

da8b497

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

Merge remote-tracking branch 'envoy/master' into upstream-filter-state

aa4c186

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

Merge remote-tracking branch 'envoy/master' into upstream-filter-state

84fcf6f

Signed-off-by: Snow Pettersen <kpettersen@netflix.com>

ggreenway approved these changes Apr 17, 2020

View reviewed changes

ggreenway merged commit 582ab4a into envoyproxy:master Apr 17, 2020

eziskind mentioned this pull request Mar 17, 2021

router: set upstream connection filter state in downstream request stream info #15543

Merged

snowp mentioned this pull request Nov 30, 2021

Proxy Protocol Listener Filter v2 metadata not available at the HTTP level. #19046

Closed

Conversation

snowp commented Mar 19, 2020

Uh oh!

snowp Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

ggreenway Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

ggreenway Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

ggreenway Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

ggreenway Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

ggreenway Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Mar 20, 2020

Uh oh!

stale bot commented Mar 27, 2020

Uh oh!

sunjayBhatia Mar 31, 2020

Choose a reason for hiding this comment

Uh oh!

snowp commented Apr 1, 2020

Uh oh!

repokitteh-read-only bot commented Apr 1, 2020

Uh oh!

snowp commented Apr 2, 2020

Uh oh!

azure-pipelines bot commented Apr 2, 2020

Uh oh!

ggreenway commented Apr 6, 2020

Uh oh!

snowp commented Apr 7, 2020

Uh oh!

snowp commented Apr 17, 2020

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants